Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Make sure that namespaces will not be purged #2963

Closed
wants to merge 1 commit into from
Closed

fix: Make sure that namespaces will not be purged #2963

wants to merge 1 commit into from

Conversation

keskad
Copy link

@keskad keskad commented Mar 22, 2022 •
edited
Loading

There was an issue reported on Slack: https://kubernetes.slack.com/archives/C9MBGQJRH/p1647955769010979

Generally I think that resources such as namespaces should be excluded from purging.
I upgraded my two clusters and was not able to reproduce the issue.

Possible Scenario

We fixed an issue that some resources were not created because of bug in jx gitops (see PR: jenkins-x-plugins/jx-gitops#835), so a new resources started appearing - for me a jx-git-operator namespace was created after cluster upgrade AND IT WAS LOOKING FINE, nothing was deleted.

 31 files changed, 4347 insertions(+), 4484 deletions(-)
 create mode 100644 config-root/cluster/namespaces/jx-git-operator.yaml

But what if somebody had different labels applied on jx-git-operator-namespace, maybe older version of Jenkins X? Then freshly created namespace maybe was different from that applied on a cluster and the old one was pruned, so the bootjob stopped and new one had no time to apply?

Resolution

Do not prune namespaces, as it is really dangerous, namespaces could be shared with other applications and should not be pruned cluster-wide.

Compromise: We do not prune anymore resources that are cluster scoped (role bindings, namespaces)

@jenkins-x-bot
Copy link
Contributor

Hi @keskad. Thanks for your PR.

I'm waiting for a jenkins-x or todo member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the jenkins-x/lighthouse repository.

@jenkins-x-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign hferentschik
You can assign the PR to them by writing /assign @hferentschik in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

keskad added a commit to keskad/jx-gitops that referenced this pull request Mar 22, 2022
@keskad
fix: Do not recreate namespace jx-git-operator
24ba6bc 
https://kubernetes.slack.com/archives/C9MBGQJRH/p1647955769010979
jenkins-x/jx3-versions#2963
Signed-off-by: Damian Kęska <372403+keskad@users.noreply.github.com>
@keskad keskad mentioned this pull request Mar 22, 2022
Merged
keskad added a commit to keskad/jx3-versions that referenced this pull request Mar 22, 2022
@keskad
feat: Double make sure the --prune will not harm, limit scope of ob…
18a1aed 
…ject types that could be deleted

jenkins-x#2963
https://kubernetes.slack.com/archives/C9MBGQJRH/p1647955769010979
keskad added a commit to keskad/jx3-versions that referenced this pull request Mar 22, 2022
@keskad
feat: Double make sure the --prune will not harm, limit scope of ob…
d921bfd 
…ject types that could be deleted

jenkins-x#2963
https://kubernetes.slack.com/archives/C9MBGQJRH/p1647955769010979
@keskad
Copy link
Author

keskad commented Mar 22, 2022 •
edited
Loading

I created also an extended proposal:
e583340 (is not a part of this Pull Request)

Please say what do you think about the concept. I think if we are deleting resources we need to wear double condoms.

Based on docs from:

@keskad
Copy link
Author

keskad commented Mar 22, 2022

/hold

Needs to be double-tested before merged.

keskad added a commit to keskad/jx3-versions that referenced this pull request Mar 22, 2022
@keskad
fix: Disable prune on cluster resources, as there are namespaces inside
e4f2ed2 
jenkins-x#2963
https://kubernetes.slack.com/archives/C9MBGQJRH/p1647955769010979
keskad added a commit to keskad/jx3-versions that referenced this pull request Mar 22, 2022
@keskad
fix: Use a prune whitelist to be double sure about removed resources
e583340 
jenkins-x#2963
https://kubernetes.slack.com/archives/C9MBGQJRH/p1647955769010979
@keskad
Copy link
Author

keskad commented Mar 22, 2022

Will extensively test tommorow (24.03.2022)

@roulettedares
Copy link
Contributor

pain

error: error pruning nonNamespaced object /v1, Kind=Namespace: namespaces "kube-system" is forbidden: this namespace may not be deleted
make: *** [versionStream/src/Makefile.mk:289: kubectl-apply] Error 1

@keskad
Copy link
Author

keskad commented Mar 25, 2022

Source changes were reverted, so this testing was not finalized on that day because of revert. Will get back to it very soon.

@keskad
Copy link
Author

keskad commented Mar 25, 2022

Related PR: #2398

Worth considering changing approach to applying changes to the cluster at all, to be more careful.

@msvticket
Copy link
Member

The main problem in Jenkins-X today is the opposite: Kubernetes resources are not removed when they should due to limitations in kubectl apply --prune. What actually often saves us is that we do support deleting namespaces and then all resources in the namespace is deleted including those kubectl apply --prune fail to delete.

I am looking at solving the problem of resources not being removed. When this is in place I think we could making it optional to not delete namespaces. Or maybe do it the other way: Make it optional to support deleting namespaces.

@msvticket
Copy link
Member

The approach here to not support deleting any cluster resources could certainly give severe unforeseen effects.

@msvticket msvticket closed this Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
do-not-merge/hold needs-ok-to-test size/XS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@keskad @jenkins-x-bot @roulettedares @msvticket